Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builder: fix image label sanity checks #249

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

lbajolet-hashicorp
Copy link
Contributor

When the image sanity checks were introduced in v1.1.7, an error slipped in as the same regex was used to validate keys and labels, however the rules are slightly different between the two, as keys are mandated to start with [a-z], while values are not.

This commit addresses this bug by changing to using two regexes, so we can still validate that both are valid before attempting to build the image, but we are not unnecessarily rejecting valid values in the map.

Closes #248

Copy link

@chrisstaite chrisstaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new regex allows empty label values which is invalid.

@lbajolet-hashicorp
Copy link
Contributor Author

@chrisstaite that is surprising, according to the GCP docs, it does state that Values can be empty, and have a maximum length of 63 characters., is this not accurate?

Copy link

@chrisstaite chrisstaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also assume lowercase ASCII (i.e. a-z) whereas the requirements from Google state lowercase letters implying lowercase non-latin characters are also allowed, specifically in UTF-8 encoding.

@chrisstaite
Copy link

chrisstaite commented Feb 13, 2025

Absolutely, I misread that.

However: "All characters must use UTF-8 encoding, and international characters are allowed. "

@lbajolet-hashicorp
Copy link
Contributor Author

You do make a point regarding lowercase letters though, I wonder what is the exact character set they allow there, I assume we can use a Unicode-friendly lowercase collation algorithm for those, but a regexp supporting those is likely tricky.
I can think of another way to do this kind of check, but in the meantime I wonder if reverting the original commit may not be better. It has the undesirable effect of rejecting an image after it's been built, but it's better than preventing builds until then.

I'll amend the PR to do that.

@chrisstaite
Copy link

You should be able to use the regex \p{Ll} although I've not tested this

@lbajolet-hashicorp
Copy link
Contributor Author

I might add that even the length check is somewhat broken in this case, as the length of the string according to their documentation is up-to 63 characters, not 63 bytes, so the length checks are likely broken too here.

@lbajolet-hashicorp
Copy link
Contributor Author

You are right though, it seems that \p{Ll} might work here, assuming the repetition works on the characters and not bytes (which I would assume is the case), we should be good to use that.
I'll experiment with that a bit, and if I cannot make it work easily, I'll resort to reverting the commit instead.

@lbajolet-hashicorp lbajolet-hashicorp force-pushed the fix_label_validation branch 2 times, most recently from 078e699 to d4e0780 Compare February 13, 2025 22:01
@lbajolet-hashicorp
Copy link
Contributor Author

@chrisstaite thanks for the pointer to \p{Ll}, this seems to be working with international lower-case characters now, hopefully this won't be a problem since I worry that between unicode versions there might be incompabtibilities in terms of determining what is a lower-case letter, though those blocks are fairly stable these days and only the astral plan tends to be updated, so I assume this will be good enough in the current state.
If you still have time, might I ask you to test it once more, and give it a quick second review?

@jkoelndorfer
Copy link

I encountered #248 today, also. It appears that this validation happens after the image build process.

If this validation is going to be performed by Packer, it would be much more useful for it to happen before the image build starts. If the build process has already completed, at that point you might as well just ask GCP to create the image. If there is an error, GCP's API will say so and that can be reported to the user.

@lbajolet-hashicorp
Copy link
Contributor Author

@jkoelndorfer this PR performs this check during the Prepare step, which happens before the builder starts, so this is indeed run before anything else. This was the intent with the change that was rolled into v1.1.7 as well, I mistakenly assumed that the check was done during this step, but it was done during the step_create_image, which as you point out, does not prevent the plugin from doing the actual build beforehand, that was my mistake, apologies for this.

If you test this change on your build and an invalid key/value, you will see that the error is immediate now.

Copy link
Contributor

@DocEmmetBrown DocEmmetBrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @lbajolet-hashicorp for quickly fixing the bug I introduced 🙏

When the image sanity checks were introduced in v1.1.7, an error slipped
in as the same regex was used to validate keys and labels, however the
rules are slightly different between the two, as keys are mandated to
start with [a-z], while values are not.

This commit addresses this bug by changing to using two regexes, so we
can still validate that both are valid before attempting to build the
image, but we are not unnecessarily rejecting valid values in the map.
Copy link

@mogrogan mogrogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@lbajolet-hashicorp lbajolet-hashicorp merged commit 9fa15ea into main Feb 14, 2025
12 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the fix_label_validation branch February 14, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 1.1.7 blocks valid image label values
5 participants